3.x: handle synchronous request retry failures#943
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR updates Sequence Diagram(s)sequenceDiagram
participant RequestHandler
participant HostConnectionPool
participant Connection
participant NextHost
RequestHandler->>HostConnectionPool: borrowConnection(...)
HostConnectionPool-->>RequestHandler: ConnectionException / BusyConnectionException
RequestHandler->>RequestHandler: logError(...)
RequestHandler->>NextHost: try next host
RequestHandler->>Connection: write(prepareAndRetry(...))
Connection-->>RequestHandler: ConnectionException / BusyConnectionException / RuntimeException
RequestHandler->>Connection: release() / release(true)
RequestHandler->>RequestHandler: retry(false, null)
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR updates the driver-core request retry internals to correctly handle synchronous transport failures that can occur before the existing async callback-based retry/failover logic is installed (notably during connection borrow and UNPREPARED re-prepare).
Changes:
- Treat synchronous
ConnectionException/BusyConnectionExceptionthrown duringHostConnectionPool.borrowConnection(...)as a host failure and continue the query plan to the next host. - Handle synchronous connection/busy failures when sending the internal
PREPAREafter anUNPREPAREDresponse by recording the host error and retrying via the normal next-host/no-host path. - Add regression tests covering (1) synchronous borrow failure failover to the next host and (2) synchronous defunct-connection failure during UNPREPARED re-prepare resulting in
NoHostAvailableExceptionwith per-host errors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
driver-core/src/main/java/com/datastax/driver/core/RequestHandler.java |
Catches sync connection/busy failures during borrow and UNPREPARED re-prepare write, logging host errors and allowing the query plan to continue. |
driver-core/src/test/java/com/datastax/driver/core/RequestHandlerTest.java |
Adds regression coverage for sync borrow failure failover and sync PREPARE-write failure during UNPREPARED recovery. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b157fd6 to
6ed5c98
Compare
| } catch (RuntimeException e) { | ||
| targetKeyspace.compareAndSet(attempt, existingAttempt); | ||
| ksFuture.setException(e); | ||
| throw e; |
There was a problem hiding this comment.
Do we need to throw? Why can't we just return future?
There was a problem hiding this comment.
Addressed in the current branch: setKeyspaceAsync now restores the target keyspace state, completes ksFuture exceptionally, and returns that future when write(...) fails instead of rethrowing.
6ed5c98 to
16f5c34
Compare
16f5c34 to
75888d3
Compare
dkropachev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@driver-core/src/main/java/com/datastax/driver/core/RequestHandler.java`:
- Around line 432-438: The borrow-connection failure handling in RequestHandler
should keep the connection error metric consistent across sync and async paths.
In the `borrowConnection` async `onFailure` callback, add the same
`metrics().getErrorMetrics().getConnectionErrors().inc()` update that the
`ConnectionException` catch block already performs, using the same
`metricsEnabled()` guard and existing logging/retry flow. This should be applied
in the `RequestHandler` failure handling code around the `borrowConnection`
callback so `connectionErrors` is incremented regardless of timing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 70c76a0a-9f79-4f79-a3be-a01e1fcfa673
📒 Files selected for processing (2)
driver-core/src/main/java/com/datastax/driver/core/RequestHandler.javadriver-core/src/test/java/com/datastax/driver/core/RequestHandlerTest.java
nikagra
left a comment
nikagra
left a comment
There was a problem hiding this comment.
Reviewed the two production-code changes and the test additions.
Production code (RequestHandler.java)
The core fix in the UNPREPARED reprepare path is correct. Before this PR, if write(connection, prepareAndRetry(...)) threw synchronously (e.g., ConnectionException("Write attempt on defunct connection")), the outer catch (Exception e) at the bottom of onSet would catch it but never call connection.release(), leaking the connection. The new inner try-catch fixes that.
Left two inline comments below.
Test code (RequestHandlerTest.java)
The should_report_no_host_when_unprepared_prepare_write_fails_synchronously test is a solid regression test for the real bug path.
One reliability concern with waitForInFlight: the assertion runs after the busy-wait loop exits, but inFlight could drop to zero between the loop condition being satisfied and the assertion. The 200 ms scassandra delay makes this practically safe, but the pattern is technically racy.
// loop exits when inFlight > 0 ...
assertThat(connection.inFlight.get()).isGreaterThan(0); // could be 0 by nowThis is unlikely to cause flakes given the 200 ms delay, but worth a note.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
driver-core/src/test/java/com/datastax/driver/core/RequestHandlerTest.java (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
ArgumentMatchersover deprecatedMatchers.
org.mockito.Matchers(Lines 25-29) has been deprecated since Mockito 2.1.0 in favor oforg.mockito.ArgumentMatchersto avoid the Hamcrest name clash, and is deprecated in order to avoid a name clash with Hamcrest org.hamcrest.Matchers class, and will likely be removed in version 3.0.♻️ Suggested import fix
-import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyInt; -import static org.mockito.Matchers.anyLong; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyLong;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@driver-core/src/test/java/com/datastax/driver/core/RequestHandlerTest.java` around lines 25 - 29, The test imports in RequestHandlerTest still use deprecated Mockito Matchers APIs; replace the org.mockito.Matchers static imports with org.mockito.ArgumentMatchers equivalents while keeping the existing Mockito usage in the test methods unchanged. Update the import block so any, anyInt, and anyLong come from ArgumentMatchers, and verify no remaining references to Matchers are left in this test class.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@driver-core/src/test/java/com/datastax/driver/core/RequestHandlerTest.java`:
- Around line 25-29: The test imports in RequestHandlerTest still use deprecated
Mockito Matchers APIs; replace the org.mockito.Matchers static imports with
org.mockito.ArgumentMatchers equivalents while keeping the existing Mockito
usage in the test methods unchanged. Update the import block so any, anyInt, and
anyLong come from ArgumentMatchers, and verify no remaining references to
Matchers are left in this test class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 219ab2ad-9938-442b-8679-599008b196e9
📒 Files selected for processing (3)
driver-core/src/main/java/com/datastax/driver/core/Connection.javadriver-core/src/main/java/com/datastax/driver/core/RequestHandler.javadriver-core/src/test/java/com/datastax/driver/core/RequestHandlerTest.java
74fe249 to
efe29bf
Compare
Fixes #941
Part of #939
RequestHandler can see request-path transport failures before the normal asynchronous callback retry path is installed.
Changes:
ConnectionException/BusyConnectionExceptionfromHostConnectionPool.borrowConnection(...)as failed host attempts so the query plan can continue to another host.PREPAREafter anUNPREPAREDresponse by releasing the connection, recording the host error, and continuing through retry/no-host handling.UNPREPAREDreprepare.Testing: